Conversation
hankem
changed the title
hankem
changed the title
|
Hey @hankem & @codecholeric, Thank you very much for this adaptation. Can you estimate when this PR will be reviewed? |
|
I'm sorry @stefan-heilig-mw; I missed your message! 🙈 Just FYI: I think that the changes only affect the build internally; ArchUnit 1.4.1 already supports JDK 25 (#1440). |
|
Thank you for your reply. We have managed to get it working since then. There was an error in our setup, which initially pointed to ArchUnit. |
hankem
changed the title
as JDK 25 will add a call to `java.util.Objects.requireNonNull` in the constructor of inner classes, cf. https://inside.java/2025/04/04/quality-heads-up/ and https://bugs.openjdk.org/browse/JDK-8351274 . Signed-off-by: Manfred Hanke <manfred.hanke@tngtech.com>
Signed-off-by: Manfred Hanke <manfred.hanke@tngtech.com>
cf. * https://docs.gradle.org/9.0.0/release-notes.html * https://docs.gradle.org/9.1.0/release-notes.html (which supports Java 25) * https://docs.gradle.org/9.2.0/release-notes.html * https://docs.gradle.org/9.3.0/release-notes.html * https://docs.gradle.org/9.4.0/release-notes.html (which supports Java 26) Signed-off-by: Manfred Hanke <manfred.hanke@tngtech.com>
Signed-off-by: Manfred Hanke <manfred.hanke@tngtech.com>
hankem
changed the title
| new ClassFileImporter().importClasses(Parent.class, Child.class) | ||
| .get(Child.class) | ||
| .getMethodCallsFromSelf()); | ||
| .getMethodCallsFromSelf() |
There was a problem hiding this comment.
It took me a really long time reproduce the second method call locally, as the build always falls back to java8 as the minimum supported version.
The easiest was in the end to set testJavaVersion=25 manually in the gradle.properties.
Is it document somewhere how to choose the java version for local execution?
| public void imports_parameter_types_of_generic_call_target_as_raw_types() { | ||
| // We cannot determine generic types of a call target from the bytecode, so the only options are to resolve the target | ||
| // and check for generic types or to treat call target parameters always as raw, same as they are in the bytecode. | ||
| // For now I have decided for the the latter. |
There was a problem hiding this comment.
very optional: remove duplicated "the"
| .get(Origin.class) | ||
| .getMethodCallsFromSelf()); | ||
| JavaMethodCall call = getOnlyElement( | ||
| new ClassFileImporter().importClasses(Origin.class, Target.class) |
There was a problem hiding this comment.
optional: keep line breaks as before
same for other tests
| .getMethodCallsFromSelf()); | ||
| .getMethodCallsFromSelf() | ||
| .stream() | ||
| .filter(call -> call.getOrigin().isMethod()) |
There was a problem hiding this comment.
this relies entirely on the fact the the new null check is added in the constructor which doesn't count as method.
I think this would be easier to understand if you write the filter in a way that makes clear that the target Objects.requireNonNull is excluded and adding a small comment that this is added in java 25+
both the bridge method and the new null check added in byte code where new to me.
As a user of ArchUnit I would be very surprised to see the new synthetic method call to Objects.requireNonNull.
Is that an expected migration effort for Java25 or do we need a special handling for this call (e.g. ignoring it)?
| .importClass(Origin.class) | ||
| .getMethodCallsFromSelf() | ||
| .stream() | ||
| .filter(c -> c.getOrigin().isMethod()) |
There was a problem hiding this comment.
if you are not extremly aware of the new null check, the logic behind this is quite surprising.
Could we filter for origin Origin.resolvesMethodCallTargetOwner instead? it would at least be clear that exactly that one method call in it is meant
or functionally equivalent: again use .getMethod("resolvesMethodCallTargetOwner").getMethodCallsFromSelf() as in other tests
| .get(Caller.class) | ||
| .getMethodCallsFromSelf() | ||
| .stream() | ||
| .filter(call -> call.getOrigin().isMethod()) |
There was a problem hiding this comment.
call.getOrigin().getName().matches("call\\d")?
you filter for the name afterwards anyway and this would free us from isMethod (meant as isNotConstructor) which seems a pretty random check to me, if I wouldn't have read the commit message
| @@ -1,7 +1,6 @@ | |||
| distributionBase=GRADLE_USER_HOME | |||
| distributionPath=wrapper/dists | |||
| distributionSha256Sum=bd71102213493060956ec229d946beee57158dbd89d0e62b91bca0fa2c5f3531 | |||
There was a problem hiding this comment.
did you drop the checksum intentionally?
according to https://gradle.org/release-checksums/ it is now for binary version 9.4.1 2ab2958f2a1e51120c326cad6f385153bb11ee93b3c216c5fccebfdfbb7ec6cb
and for the wrapper 55243ef57851f12b070ad14f7f5bb8302daceeebc5bce5ece5fa6edb23e1145c (I verified that your new wrapper file has the correct checksum)
If we actually check it somewhere in the pipeline, I'd prefer to keep it, otherwise, deletion is fine
3 participants
Update Gradle Wrapper from 8.14.3 to 9.3.1
Release notes:
Also run tests with JDK 25
Release notes: